Feat: Added extra argument and test for the save option for text files - #9142
Conversation
perminder-17
left a comment
perminder-17
left a comment
There was a problem hiding this comment.
Hi, thanks for contributing!
Could you please provide a little more context about the issue? I just came across it, but I’m not fully clear on what was missing and how this PR fixes the issue.
|
Hi @perminder-17 , thank you for the review and the doubt regarding the PR : The main consequence is that text files saved via save() would always use \n line endings, even when the caller explicitly asked for CRLF (true). That can cause Windows-style line-ending expectations to fail, especially when files are opened or processed by tools that rely on \r\n for compatibility. |
| return; | ||
| case 'txt': | ||
| fn.saveStrings(args[0], args[1], args[2]); | ||
| fn.saveStrings(args[0], args[1], args[2], args[3]); // Takes the third argument |
There was a problem hiding this comment.
Please remove the comment here.
There was a problem hiding this comment.
@limzykenneth Thank you for the feedback, removed the comment
There was a problem hiding this comment.
Should it also apply here as well?
There was a problem hiding this comment.
Yes, I have updated the argument list with the required unit tests
… tests and the ffedback received
… into feat/save-clrf
|
@limzykenneth Thank you for the feedback and the questions asked. I have fixed the comment line and also added the third argument for the default case with the required tests. If any other changes, do reach out to me, would be happy to fix it or clarify any queries. |
| default: | ||
| if (args[0] instanceof Array) { | ||
| fn.saveStrings(args[0], args[1], args[2]); | ||
| fn.saveStrings(args[0], args[1], args[2], args[3]); |
There was a problem hiding this comment.
Just looked into it a bit more, the save() function itself is not meant to have 4 arguments, which means args[3] should never be a thing. The way it works is by detecting the file extension being txt and passing it to saveStrings(). ie. this is a call arguments positional issue rather than adding another argument to the call, if that made sense.
There was a problem hiding this comment.
…s and redirected to saveStrings
|
@limzykenneth Thanks for the clarification. After reading the docs found out that save is not meant to take the 4th parameter, but the clrf flag is passed as the third argument. I have updated the text-file dispatch to pass args[2] through to saveStrings() as the isCRLF flag, and adjusted the regression test to match the documented API shape. I also confirmed that the array fallback path was using the same issue pattern, so it now follows the same contract as the explicit .txt case. |
|
Looks good. Thanks! |
|
@limzykenneth Thanks for the merge and helpful feedback! Excited to contribute more to this project soon. |
3 participants
Resolves #9141
Changes:
Added third argument for saving txt files along with the required tests
PR Checklist
npm run lintpasses